Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

main: Ignore SIGPIPE when printing version #3203

Merged
merged 1 commit into from
Feb 27, 2024

Conversation

dbnicholson
Copy link
Member

In order to do a runtime feature check, ostree --version can be piped to grep or similar. However, if the read end of the pipe doesn't read all of the output, ostree will receive SIGPIPE when trying to write output. Ignore it so that ostree still exits successfully in that case.

In order to do a runtime feature check, `ostree --version` can be piped
to `grep` or similar. However, if the read end of the pipe doesn't read
all of the output, `ostree` will receive `SIGPIPE` when trying to write
output. Ignore it so that `ostree` still exits successfully in that
case.
@dbnicholson
Copy link
Member Author

I couldn't actually test this on my local machine since I could never trigger the SIGPIPE before, but I think it's the right thing to do. It actually might not be a bad thing to do this all the time (not just under --version) in the CLI since there are several subcommands that have grepable output.

@cgwalters
Copy link
Member

It actually might not be a bad thing to do this all the time (not just under --version) in the CLI since there are several subcommands that have grepable output.

Hmmm...I am wondering if there is precedent for this in other projects. GLib is not alone in changing SIGPIPE by default - so does Rust for example.

@cgwalters
Copy link
Member

And yeah per my comment here...fun fact is that glib changes it in the class constructor for GSocket. Not that we're going to make a socket in the printing code for --version, but still...funky action-at-a-distance there.

@dbnicholson
Copy link
Member Author

Because I didn't actually understand this behavior, I decided to look it up. Per pipe(7), if you don't ignore SIGPIPE then your process will receive SIGPIPE when writing to a pipe with no reader. If you do ignore SIGPIPE, then writes will fail with EPIPE when there's no reader. In the case of g_print, by default it uses fputs and essentially ignores errors.

@dbnicholson
Copy link
Member Author

That rust issue has a link to an interesting interpretation of SIGPIPE on stackoverflow. The point of SIGPIPE is to "automatically" kill programs in the pipeline when a downstream reader is done. It's ugly but kinda makes sense. It actually seems like the more correct thing would be for the pipefail bash option to treat processes that exit due to SIGPIPE as equivalent to exiting successfully. There's another interesting discussion about that.

@cgwalters cgwalters merged commit 1a54d94 into ostreedev:main Feb 27, 2024
24 checks passed
@dbnicholson dbnicholson deleted the version-sigpipe branch February 27, 2024 17:32
@@ -510,6 +511,11 @@ ostree_option_context_parse (GOptionContext *context, const GOptionEntry *main_e

if (opt_version)
{
/* Ignore SIGPIPE so that piping the output to grep or similar
* doesn't cause the process to fail. */
if (signal (SIGPIPE, SIG_IGN) == SIG_ERR)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually wait though, shouldn't we be setting it to SIG_DFL here so we get killed by SIGPIPE, which the shell is expecting?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, that's exactly what's happening now. The default disposition for SIGPIPE is to kill the process. The process gets killed and the shell sets the exit status to 128 + signum = 141. The pipefail feature says that any process with a non-0 exit status is considered failed. I think it would be better if bash saw the SIGPIPE triggered exit and treated it as a successful exit, but that's not what it does. See https://lists.nongnu.org/archive/html/bug-bash/2017-03/msg00179.html for one of the util-linux maintainers unsuccessfully trying to convince the bash maintainers to do that.

By ignoring the signal, the process won't be killed. Instead, all the writes will fail with EPIPE, which g_print will ignore. That's not so nice since it will keep trying to write data even though there's no reader. However, since the opt_version output is small and controlled, it's no big deal.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, thanks. So basically there's no way to get strict error handling right now for pipes that works reliably.

Yeah. I think the answer here is to not use shell script...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah. I think the answer here is to not use shell script...

Exactly. For me it's kinda sad because my entry point to software was elaborate shell scripts to do various things. But after being bitten by one too many of these gotchas I've mostly stopped writing them unless they're very simple.

jlebon added a commit to jlebon/os that referenced this pull request Apr 15, 2024
Unlike `grep`, `grep -q` will exit 0 as soon as a match is found. This
will cause whatever is writing into `grep` to hit `SIGPIPE`. And if it's
not equipped to handle that signal, it'll be terminated and the overall
if-condition will always fail due to `-o pipefail`.

See also ostreedev/ostree#3203.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants